Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: override get/setproperty and propertynames #517

Merged
merged 26 commits into from
Jan 23, 2019

Conversation

carstenbauer
Copy link
Contributor

The intent of this PR is to use 0.7/1.0's getproperty functionality to make o.foo behave as in Python, i.e. to map it to the current o[:foo].

@carstenbauer
Copy link
Contributor Author

Minimal example:

@pyimport numpy as np
np.random.rand() #works now

Autocompletion works as well, i.e. np.random.<tab> gives a list of all available members of the python object.

@carstenbauer carstenbauer changed the title Override getproperty and propertynames Override get/setproperty and propertynames Aug 12, 2018
@stevengj
Copy link
Member

stevengj commented Aug 15, 2018

Thanks. I was planning on waiting a little while to do this, so that we can:

  • Drop 0.6 support.
  • Deprecate all the old getindex methods. (Eventually they will be made equivalent to o[i] in Python once the deprecations are removed.)
  • Deprecate @pyimport and pywrap since you can now just do np = pyimport("numpy")

src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
@carstenbauer
Copy link
Contributor Author

carstenbauer commented Aug 22, 2018

Ok, I tried to implement all your comments. Replacing o.o by a new PyPtr(o::PyObject) method effected many files (in a trivial way). Locally, it passes all tests. (I'll add some new specific tests later)

I look forward to comments and suggestions.

benchmarks/pywrapfn.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/PyCall.jl Outdated Show resolved Hide resolved
src/conversions.jl Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Aug 23, 2018

Codecov Report

Merging #517 into master will decrease coverage by 1.09%.
The diff coverage is 46.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #517     +/-   ##
=======================================
- Coverage   54.09%    53%   -1.1%     
=======================================
  Files          19     19             
  Lines        1525   1566     +41     
=======================================
+ Hits          825    830      +5     
- Misses        700    736     +36
Impacted Files Coverage Δ
src/exception.jl 88.05% <ø> (+1.29%) ⬆️
src/pyinit.jl 74.54% <0%> (ø) ⬆️
src/gui.jl 0% <0%> (ø) ⬆️
src/pydates.jl 58.82% <0%> (-3.68%) ⬇️
src/pyfncall.jl 67.64% <0%> (ø) ⬆️
src/io.jl 36.76% <100%> (-2.92%) ⬇️
src/callback.jl 89.28% <100%> (ø) ⬆️
src/serialize.jl 100% <100%> (ø) ⬆️
src/PyCall.jl 59.17% <48.48%> (-1.96%) ⬇️
src/pytype.jl 60.56% <60%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9cca81...77736c8. Read the comment docs.

@carstenbauer
Copy link
Contributor Author

BTW, regarding your third point above, I really like the "@pyimport numpy as np" syntax because it's basically the same in python. Maybe we can keep it as syntactic sugar?

@oxinabox
Copy link

pywrap(::PyObject)::Module can be deprecated with this.
Since that was just a hack to pretend to have . overloading.

(I can't workout if that has been done, so many things in this package are called py.*wrap.*)

@stevengj
Copy link
Member

stevengj commented Aug 27, 2018

basically the same in python. Maybe we can keep it as syntactic sugar?

No. The only reason that sugar exists was the lack of dot overloading. Now you can do const np = pyimport("numpy") if you want, and there is no benefit to a second syntax.

@carstenbauer
Copy link
Contributor Author

I understand why the macro was necessary. But as I said, I would see a benefit in the second syntax because it's close to what you do in python (and this is a python interface). I won't push this point any further though.

@carstenbauer carstenbauer changed the title Override get/setproperty and propertynames WIP: override get/setproperty and propertynames Sep 8, 2018
@carstenbauer
Copy link
Contributor Author

Added some simple tests. What should I do next to move forward with this?

@Datseris
Copy link
Contributor

perfect, thanks for the amazing work on this PR!

Notice that most of my warnings come from files within PyPlot so someone else should open a PR there.

src/PyCall.jl Outdated Show resolved Hide resolved
@carstenbauer
Copy link
Contributor Author

carstenbauer commented Jan 22, 2019

Ok, AFAICS this PR should now be technically mergable (let's see if the tests pass on Linux and MacOS as well). I fixed all the merge conflicts, deprecated the getindex and setindex! methods and fixed all corresponding internal deprecation warnings (at least those that show up in the tests).

However,

  • the README.md should be revisited. I made some basic adjustments but I guess a proper revisit/rewrite of the relevant section would be appropriate. As I'm not a native english speaker, it might make sense for someone else to take care of that?
  • I haven't tested the getindex -> getproperty changes in gui.jl because I'm on Windows.

Since it's already after midnight, I guess it's time to go to bed :)

@carstenbauer
Copy link
Contributor Author

The single appveyor fail seems to be a random network issue.

@stevengj
Copy link
Member

AppVeyor failure is an unrelated network glitch.

@stevengj stevengj merged commit e4035b8 into JuliaPy:master Jan 23, 2019
@stevengj stevengj mentioned this pull request Jan 23, 2019
5 tasks
@stevengj
Copy link
Member

See #629 for remaining steps.

@stevengj stevengj mentioned this pull request Jan 23, 2019
@tkf
Copy link
Member

tkf commented Jan 24, 2019

Big thanks to @crstnbr for the big merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants